Documentation for this PR has been generated and is available at: https://n0-computer.github.io/iroh/pr/3523/docs/iroh/

Last updated: 2025-10-17T08:25:55Z

Netsim report & logs for this PR have been generated and is available at: LOGS
This report will remain available for 3 days.

Last updated for commit: 1842948

Hey, so I pushed a commit that includes removing .relay_mode(RelayMode::Disabled) where we are now using an empty_builder(), since that already includes RelayMode::Disabled and added using an empty_builder in places that shouldn't have discovery enabled in the test (from what I can tell).

If I've misunderstood the purpose of a test feel free to revert or change.

Two concerns about this PR:

1. users are not going to be used to having discovery on by default
2. it's not immediately obvious that if you use an empty_builder and then add discovery services yourself, you may be foot-gunning yourself if you don't also add a specific relay mode to the endpoint builder as well, since most of our discovery really depends on being connected to a relay mode, and empty_builder sets the relay mode to RelayMode::Disabled

The first issue is just about communicate, and I think folks will just get used to it.

The second one I'm more concerned about because it's easy to just not realize what you are doing. This may also be solved a bit by documentation (for example, in the empty_builder method, mentioning that if you plan to add a discovery service you may also want to add a relay mode to the builder before binding.)

I think we need a bit of an answer around the second point before we can merge. We can decide it's enough to just communicate this through docs, or we can decide that's too big of a foot-gun and need to change some things, or we can decide it doesn't actually matter, or some other fourth option.

One general thought:

• for clarity, we may want to make some presets in our test utils, that makes it obvious what the expectations are for the test, even if the preset is really just equivalent to Endpoint::empty_builder() & feels redundant.
  This may help if we:
  a) change what the defaults are in APIs like empty_builder or Endpoint::bind & then need to search each test and make sure we update accordingly
  b) just make it really clear when setting up the test, what the expectations are for endpoint at a glance.

Just a thought!

@ramfox I have pushed a version that enforces choosing a RelayMode when constructing an empty builder, in many use cases this is the same or less code than before, and only in the full empty version a little bit more verbose, which seems fine given the better understanding this lends to the reader and writer of the code